-
Notifications
You must be signed in to change notification settings - Fork 282
Add JSON Schema Definition for gen_ai.tool.definitions #2942
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add JSON Schema Definition for gen_ai.tool.definitions #2942
Conversation
Change-Id: I63f11ca8081f71b55b793ec88f35ef64bbace8e6 Co-developed-by: Cursor <[email protected]>
Change-Id: Ib6133b3019195e4fa9a54d329ff4b891a281d208 Co-developed-by: Cursor <[email protected]>
76f99f0 to
2a15973
Compare
|
This is the continued PR of #2793 |
Change-Id: Ie116f8bcb9e758b026596b797569b27496b35521 Co-developed-by: Cursor <[email protected]>
Change-Id: If96268fc1b4d3cd64c0a10437a968cbe5ec83d98 Co-developed-by: Cursor <[email protected]>
Change-Id: Ic5480fbdb1058a2e1ace084c8ce3735484f91c06 Co-developed-by: Cursor <[email protected]>
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "class FunctionToolDefinition(BaseModel):\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could it extend GenericToolDefinition? then it would reuse type, name and description.
| It's expected to be an array of objects, each representing a tool definition, | ||
| and the structure of the array is expected to match the [Tool Definitions JSON Schema](/docs/gen-ai/gen-ai-tool-definitions.json). | ||
| In case a serialized string is available to the instrumentation, the instrumentation | ||
| SHOULD do the best effort to deserialize it to an array. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| It's expected to be an array of objects, each representing a tool definition, | |
| and the structure of the array is expected to match the [Tool Definitions JSON Schema](/docs/gen-ai/gen-ai-tool-definitions.json). | |
| In case a serialized string is available to the instrumentation, the instrumentation | |
| SHOULD do the best effort to deserialize it to an array. | |
| Instrumentations MUST follow [Tool Definitions JSON Schema](/docs/gen-ai/gen-ai-tool-definitions.json) |
There is no need to mention array - it's already in the definition. Also no need to mention deserialization - it's already explicitly required by a MUST.
This is also consistent with how we define input and output messages.
| " \"\"\"\n", | ||
| " type: str = Field(description=\"The type of the tool.\")\n", | ||
| " name: str = Field(description=\"The name of the tool.\")\n", | ||
| " description: str = Field(None, description=\"The description of the tool.\")\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it a generic property? I don't see it being used on most tools - https://platform.openai.com/docs/api-reference/responses/create#responses_create-tools, let's not add it as generic?
| If instrumentations can reliably deserialize and extract the tool definitions, | ||
| it's RECOMMENDED to only populate required fields (`name`, `type`) of the definition | ||
| objects by default. Otherwise, it's NOT RECOMMENDED to populate it by default. | ||
| Instrumentations MAY provide a way to enable populating this attribute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, just more concise rephrasing:
| If instrumentations can reliably deserialize and extract the tool definitions, | |
| it's RECOMMENDED to only populate required fields (`name`, `type`) of the definition | |
| objects by default. Otherwise, it's NOT RECOMMENDED to populate it by default. | |
| Instrumentations MAY provide a way to enable populating this attribute. | |
| It’s RECOMMENDED to populate all required attributes on each tool | |
| definition by default. Instrumentations MAY allow to enable populating | |
| non-required properties. |
(instrumentations that can't "reliably deserialize and extract the tool definitions" would not populate the attribute at all)
| @@ -1 +1 @@ | |||
| # Use this changelog template to create an entry for release notes. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For google-genai it's possible to pass a tool to the model which is just a description of the function and it's parameters, and then in the response the model will say whether and how the application can invoke the function -- see https://googleapis.github.io/python-genai/#manually-declare-and-invoke-a-function-for-function-calling -- i'm wondering if and how we should capture this type of tool definition ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DylanRussell Sure, that's the common way to request model with tools definitions. So for instrumentations, I believe we should wrap the LLM invocation and you will capture the tools definitions as the parameters in your decorator.
For example:
# you could wrap this function, and capture 'tools' from 'config' in kwargs
response = client.models.generate_content(
model='gemini-2.5-flash',
contents='What is the weather like in Boston?',
config=types.GenerateContentConfig(
tools=[tool],
),
)
Fixes #2721 #1835
Changes
Add JSON schema definition for gen_ai.tool.definitions.
Important
Pull requests acceptance are subject to the triage process as described in Issue and PR Triage Management.
PRs that do not follow the guidance above, may be automatically rejected and closed.
Merge requirement checklist
[chore]